hrw4u: enforce sandbox policy on value functions - #13674
masaori335 wants to merge 2 commits into
Conversation
The sandbox `functions` deny/warn list was only consulted for statement functions, so an entry naming a function used as a value in an expression was accepted by the loader and then silently ignored. `access()`, which reaches the filesystem, was among them. The split between the two resolvers is an implementation detail; the grammar has one `functionCall` rule and the policy YAML exposes one `functions` category, so both now go through the same check.
There was a problem hiding this comment.
🟡 Changes recommended
Address the critical interpolation denial path, moderate warning-handling issue, and documentation omission.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request extends HRW4U sandbox policy enforcement to functions used as expression values.
Changes:
- Applies deny/warn policies during value-function resolution.
- Adds warning and denial regression fixtures.
- Updates the sandbox schema and administrator documentation.
File summaries
| File | Summary |
|---|---|
tools/hrw4u/tests/data/sandbox/warned-value-function.warning.txt |
Warning expectation fixture. |
tools/hrw4u/tests/data/sandbox/warned-value-function.sandbox.yaml |
Warning policy configuration. |
tools/hrw4u/tests/data/sandbox/warned-value-function.output.txt |
Expected warned-function output. |
tools/hrw4u/tests/data/sandbox/warned-value-function.input.txt |
Warned value-function input case. |
tools/hrw4u/tests/data/sandbox/warned-value-function.ast.txt |
Expected AST for the warning case. |
tools/hrw4u/tests/data/sandbox/denied-value-function.sandbox.yaml |
Denial policy configuration. |
tools/hrw4u/tests/data/sandbox/denied-value-function.input.txt |
Denied value-function input case. |
tools/hrw4u/tests/data/sandbox/denied-value-function.error.txt |
Expected denial error. |
tools/hrw4u/tests/data/sandbox/denied-value-function.ast.txt |
Expected AST for the denial case. |
tools/hrw4u/src/symbols.py |
Enforces value-function policies; warning buffering and interpolation denial handling require changes. |
tools/hrw4u/schema/sandbox.schema.json |
Adds value-function names to the sandbox policy schema. |
doc/admin-guide/configuration/hrw4u.en.rst |
Documents the expanded policy, but omits two statement functions. |
Review details
Suppressed comments (1)
tools/hrw4u/src/symbols.py:184
- The policy warning is buffered before
validator(args)runs. If a warned value function has invalid arguments, that validator raises andvisitFunctionCallnever reaches its warning drain, so the warning can be lost or attached to a later function's source context. Store the warning only after validation succeeds (while keeping the denial check before validation).
self._collect_warning(self._sandbox.check_function(func_name))
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Both are statement functions in their own right — `set-body("x")` and
`set-body-from-file("/p")` compile — and the schema enum already accepts
them, but neither the sandbox function table nor the operator mapping
table listed them all. Both tables now match STATEMENT_FUNCTION_MAP.
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
The core change is right and I want it — resolve_function now runs check_function the same way resolve_statement_func does, the schema enum is exactly FUNCTION_MAP union STATEMENT_FUNCTION_MAP (21 names, checked programmatically), the doc table matches, the new fixtures are auto-collected by test_sandbox.py, and there is no double-check or double-warning since statement calls never route through resolve_function.
Holding on one thing, because it leaves the hole this PR is closing partly open:
tools/hrw4u/src/visitor.py:233 — _substitute_strings catches bare Exception, which swallows SandboxDenialError. A function that is only reached through a string interpolation — "{func()}" — therefore still escapes policy: the denial is raised, caught, and the sandbox message is never set. The new denied-value-function fixture does not catch this because access(...) fires first and the test passes for the wrong reason. Worth letting SandboxDenialError (and probably the other hrw4u error types) propagate, plus a fixture whose only denied construct is inside an interpolation.
This is the same pattern as #13675: the policy was being enforced per surface spelling rather than per semantics, so every alternate way to reach the construct is a separate bypass. Interpolation is one more alternate way.
Two smaller things while you are in here, neither blocking:
tools/hrw4u/src/sandbox.py:97—functionsentries are still not validated at load, unlikelanguageandmodifiers, and nothing validates the YAML againstsandbox.schema.json. A misspelled function name in a policy is silently ignored, which is the same failure class this PR fixes — an operator thinks they denied something and did not. Validating against the tables would also stop schema/table drift.tools/hrw4u/src/symbols.py:184— the warning is collected before argument validation runs. If the validator raises,visitFunctionCall's trap skips_drain_resolver_warnings, so the pending warning is later attributed to an unrelated source location or dropped entirely.
Caveat: ANTLR and the antlr4 Python runtime are not available on this machine, so this was a static review — I could not run the suite.
The sandbox
functionsdeny/warn list was only consulted for statement functions, so an entry naming a function used as a value in an expression was accepted by the loader and then silently ignored.access(), which reaches the filesystem, was among them.The split between the two resolvers is an implementation detail; the grammar has one
functionCallrule and the policy YAML exposes onefunctionscategory, so both now go through the same check.